feat: cross-platform force-kill primitive for stuck PHP threads#2365
feat: cross-platform force-kill primitive for stuck PHP threads#2365nicolas-grekas wants to merge 1 commit intophp:mainfrom
Conversation
a7be2c7 to
692ee4c
Compare
withinboredom
left a comment
There was a problem hiding this comment.
Looks pretty good. FYI: bugs like php/php-src#21267 mean that sometimes JIT just will never hit these vm breakpoints. So, be prepared for bug reports that aren't related to this change, but are due to JIT.
|
Good work! Shouldn't this code be directly on php-src (TSRM)? It could be useful in other contexts than FrankenPHP. |
|
@dunglas dunno in the future, but at the moment, this allows providing the capablity to all versions of PHP, without having to wait for an hypothetical merge in 8.6+. |
The two don't exclude each other, it would actually help getting this upstreamed because it will open the doors to more calls being switched to alertable. Once that lands in master, we can backport it in FrankenPHP for 8.4+. |
Introduces a self-contained primitive that unblocks a PHP thread stuck in a blocking call (sleep, synchronous I/O, etc.) so the graceful drain used by RestartWorkers, DrainWorkers, and Shutdown makes progress instead of hanging for the duration of the block. The primitive is useful on its own and gives follow-up graceful-shutdown work a reviewed foundation to build on. Design: each PHP thread, at boot from its own TSRM context, hands a force_kill_slot (pointers to its own EG(vm_interrupt) and EG(timed_out) atomic bools, plus pthread_t / Windows HANDLE for the wake-up) back to Go via go_frankenphp_store_force_kill_slot. The slot lives on phpThread and is protected by a per-thread RWMutex so the zero-and-release path at thread exit cannot race an in-flight kill. From any goroutine, Go passes the slot back to frankenphp_force_kill_thread, which stores true into both bools (waking the VM at the next opcode boundary and routing through zend_timeout -> "Maximum execution time exceeded") and then delivers a platform-specific wake-up: - Linux/FreeBSD: pthread_kill(SIGRTMIN+3) with a no-op handler installed via pthread_once, SA_ONSTACK, no SA_RESTART. Signal delivery causes any in-flight blocking syscall to return EINTR. - Windows: CancelSynchronousIo + QueueUserAPC covers alertable I/O and SleepEx. Non-alertable Sleep (including PHP's usleep) stays uninterruptible. - macOS: atomic-bool-only path. Threads stuck in blocking syscalls wait to return on their own. JIT caveat: under the OPcache JIT some hot code paths skip vm_interrupt checks (see php-src#21267), so a pure-PHP busy loop under JIT may not observe the store and will fall through to the abandon path below. Drain flow: - worker.go: drainWorkerThreads waits drainGracePeriod (5s) for each drained thread to reach Yielding; then arms force-kill on stragglers and waits forceKillDeadline (5s) more. Threads still stuck past that are abandoned rather than hanging the drain forever. - drainWorkerThreads returns (drained, abandoned). RestartWorkers puts drained threads back to Ready and abandoned ones into the new state.Abandoned (handlers treat it like ShuttingDown on next callback) so an abandoned thread that finally unwinds exits instead of re-entering the serve loop under stale request state. Abandoned threads are also filtered out of worker.threads immediately so isAtThreadLimit and the scaler see accurate capacity; the matching deactivateThreads cleanup drops Abandoned/Done entries from the auto-scaling slice so they do not permanently consume a global scaling slot. If any were abandoned, RestartWorkers returns errIncompleteRestart wrapped with abandoned/restarted counts - admin endpoint and watcher surface it. - phpthread.go: phpThread.shutdown mirrors the same grace + force-kill + abandon pattern so Shutdown cannot hang on an uninterruptible blocking call either. RequestSafeStateChange and shutdown's fast-fail WaitFor both accept Abandoned so Shutdown racing a RestartWorkers that marks a thread Abandoned does not park on a transition that will never come. Lifecycle hardening: Shutdown intentionally leaves phpThreads allocated and thread_metrics alive - an abandoned thread that eventually unwinds still calls through the SAPI and lifecycle callbacks which index those structures. initPHPThreads blocks on a package-level sync.WaitGroup (Add on every php_thread entry, Done on every exit path, routed through a single goto exit: label in php_thread so future return paths cannot silently leak an Add) so the next Init cycle cannot reassign them out from under a lingering abandoned thread, then frees the previous allocation inside frankenphp_init_thread_metrics before allocating fresh. A dedicated C-side atomic (shutdown_in_progress, toggled by frankenphp_set_shutdown_in_progress) is the signal the unhealthy-thread restart path uses to refuse respawning past Shutdown. - go_frankenphp_store_force_kill_slot / clear_force_kill_slot / on_thread_shutdown: take the per-thread write lock; clear runs before ts_free_thread on both healthy and unhealthy exit paths so the captured &EG() pointers are zeroed before their backing storage is freed. - php_thread unblocks FRANKENPHP_KILL_SIGNAL with pthread_sigmask at startup so Go's runtime signal mask cannot silently drop deliveries. Safe main-thread teardown after abandonment: an abandoned thread that unwinds after sapi_shutdown / tsrm_shutdown would touch freed TSRM storage (ts_free_thread) and torn-down SAPI (php_request_shutdown via zend_catch, ub_write, etc). php_main now calls back into Go through go_frankenphp_can_teardown after the main thread's Done signal; Go does a bounded wait (mainThreadShutdownDeadline, 5s) on the lingeringThreads WaitGroup. If every thread has exited, teardown runs normally; on timeout Go logs a warning and returns false, and the C side skips frankenphp_sapi_module.shutdown / sapi_shutdown / tsrm_shutdown entirely. PHP's SAPI/TSRM state then leaks until process exit, which is the safe outcome: a subsequent Shutdown after an errIncompleteRestart can no longer crash on a late-unwinding abandoned thread. Callers that want a fully-clean process after observing errIncompleteRestart should terminate rather than re-Init, since a skipped teardown leaves SAPI un-torn-down and the next Init would try to initialise on top of it. initPHPThreads now enforces this directly: it latches teardownSkipped when the bounded wait expires and returns ErrTeardownSkipped on any subsequent Init, so embedders cannot silently re-initialise on top of never-torn-down SAPI/TSRM. - worker_test.go + testdata/worker-sleep.php: the regression test drives the full path via a request marker file so it only arms RestartWorkers once the worker is proven parked in sleep(), then asserts both the bounded elapsed time and that the "should not reach" line after sleep never runs (which would indicate the VM interrupt was never observed). RestartWorkers now returns an error - a source-compatible Go API change (callers that ignored it still compile) but worth noting for embedders.
|
PR should be ready! It took me a while to get it correct. PR description is updated. Lots of comments in the patch; let me know if that's too much. |
First step of the split suggested in #2287: land the force-kill
infrastructure as a standalone, reviewable primitive independent of
background workers. Useful on its own, and gives follow-up
graceful-shutdown work a reviewed foundation.
Design
Each PHP thread, at boot from its own TSRM context, hands a
force_kill_slot(pointers to itsEG(vm_interrupt)andEG(timed_out)atomic bools, plus
pthread_t/ WindowsHANDLE) back to Go viago_frankenphp_store_force_kill_slot. The slot lives onphpThread,protected by a per-thread
RWMutexso the zero-and-release path atthread exit cannot race an in-flight kill.
frankenphp_force_kill_threadstores
trueinto both atomic bools (waking the VM at the next opcodeboundary, routing through
zend_timeout-> "Maximum execution timeexceeded"), then delivers a platform-specific wake-up:
pthread_kill(SIGRTMIN+3)with a no-op handlerinstalled once via
pthread_once,SA_ONSTACK, noSA_RESTART.Signal delivery returns any in-flight blocking syscall with
EINTR.CancelSynchronousIo+QueueUserAPCcovers alertableI/O and
SleepEx. Non-alertableSleep(including PHP'susleep)stays uninterruptible.
wait to return on their own.
Reserved signal:
SIGRTMIN+3. A PHP script that callspcntl_signal(SIGRTMIN+3, ...)clobbers this. Embedders whose own Gocode uses
SIGRTMIN+3must patch it here. glibc NPTL reservesSIGRTMIN..SIGRTMIN+2, so the offset cannot go lower.JIT caveat: under OPcache JIT some hot paths skip the
vm_interruptcheck (see php-src#21267).A JIT busy loop may not observe the atomic-bool store and falls through
to the abandon path.
Drain flow
drainWorkerThreadswaitsdrainGracePeriod(5s) for each thread toreach
Yielding, then arms force-kill on stragglers and waitsforceKillDeadline(5s) more. Threads still stuck past that areabandoned rather than hanging the drain forever:
state.Abandoned; handlerstreat it like
ShuttingDownon the next callback so a late-unwindingabandoned thread exits the serve loop instead of running a request
with stale context.
worker.threadsimmediately soisAtThreadLimitand the scaler see accurate capacity; theauto-scaling slice's cleanup drops Abandoned/Done entries too.
RestartWorkersnow returnserrIncompleteRestartwrapped withabandoned/restarted counts; admin endpoints and the watcher surface
it.
phpThread.shutdownmirrors the same grace + force-kill + abandonpattern so
Shutdowncannot hang on an uninterruptible call either.Safe main-thread teardown after abandonment
An abandoned thread that unwinds after
sapi_shutdown/tsrm_shutdownwould touch freed TSRM storage (
ts_free_thread) and torn-down SAPI(
php_request_shutdownviazend_catch). After the main thread's Donesignal,
php_maincallsgo_frankenphp_can_teardown, which does abounded wait (
mainThreadShutdownDeadline, 5s) on thelingeringThreadsWaitGroup. On timeout, teardown is skipped entirely: PHP's SAPI/TSRM
state leaks until process exit - the safe outcome. To prevent silent
corruption on re-Init after a skipped teardown,
initPHPThreadslatchesa
teardownSkippedflag and returns the newErrTeardownSkippedsentinel. Embedders that observe
errIncompleteRestartand want tocontinue serving should terminate the process rather than call Init
again.
Lifecycle hardening
phpThreadsandthread_metricsare intentionally left allocatedacross Shutdown so abandoned threads' late callbacks index safely.
initPHPThreadsblocks onlingeringThreads(incremented on everyphp_threadentry, decremented at the singlegoto exit:label) sothe next Init cycle cannot reassign them out from under a lingering
thread, then frees the previous
thread_metricsbefore reallocating.shutdown_in_progressatomic gates the unhealthy-thread respawn pathoff once Shutdown starts.
RequestSafeStateChangeandphpThread.shutdown's fast-fail acceptAbandonedso Shutdown cannot park on a Restarting -> Abandonedtransition.
Testing
TestRestartWorkersForceKillsStuckThreaddrives the full path via amarker file so
RestartWorkersonly arms once the worker is provenparked in
sleep(), then asserts bounded elapsed time and that thepost-sleep line never runs.
API note
RestartWorkersnow returnserror(wrapserrIncompleteRestart).Source-compatible - callers that ignored it still compile - but worth
flagging for embedders. New exported
ErrTeardownSkippedsentinel.